-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds support for trogon. #870
base: vara-dev
Are you sure you want to change the base?
Conversation
Most vara-* commands now support `tui` as a subcommand which opens a text-based UI for selecting command line parameters.
…handling. Many of the checks are not necessary anymore because our config management takes care of managing folders much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand this correctly, this is "opt-in" as in one can use the CLI as is, without changes and if wanted with the clicky UI?
PS: also some tests seem not to be happy :D
get_value_or_default( | ||
vara_cfg()["paper_config"], "folder", | ||
str(get_varats_base_folder()) | ||
) | ||
) | ||
if not (pc_folder_path.exists() and pc_folder_path.is_dir()): | ||
LOG.error( | ||
f"Paper config folder not set: {pc_folder_path} " | ||
"(Path does not exist or is no directory)." | ||
) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we no longer need the checks here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because by now we take care of creating these directories whenever we save the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the config folder has a sane default value set.
Yes, this basically adds a new subcommand
I have no idea why. That test initially also failed on my machine but for a different reason. All tests pass when I execute them locally. |
The reason for the failing test was that the CLI parameters are generated at parse-time. Since the test modified the available choices of such an option, we need to reload the driver module for that change to take effect at the CLI level.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## vara-dev #870 +/- ##
============================================
+ Coverage 68.43% 68.45% +0.02%
============================================
Files 340 340
Lines 26627 26635 +8
============================================
+ Hits 18221 18233 +12
+ Misses 8406 8402 -4 ☔ View full report in Codecov by Sentry. |
Most vara-* commands now support
tui
as a subcommand which opens a text-based UI for selecting command line parameters.Some limitations apply:
vara-run
is not supported because arguments do not play nice with command groups in clickvara-plot
andvara-table
require support forclick.MultiCommand
(Add support for click.MultiCommand Textualize/trogon#71)trogon
contains some more small bugs that limit usability. I opened PRs for them, but until they are merged, you can use the following branch for testing: https://github.com/boehmseb/trogon/tree/trogonFixes